Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor updates to documentation #189

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Minor updates to documentation #189

merged 2 commits into from
Aug 2, 2024

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Aug 2, 2024

  • Clarification and typo fixes

- Clarification and typo fixes
@jonnew jonnew requested a review from bparks13 August 2, 2024 15:13
@jonnew jonnew added the documentation Improvements or additions to documentation label Aug 2, 2024
Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'll go ahead and implement the change listed and then we can merge the PR

OpenEphys.Onix1/MultiDeviceFactory.cs Outdated Show resolved Hide resolved
@bparks13 bparks13 self-requested a review August 2, 2024 20:43
@bparks13 bparks13 merged commit 7cb45a2 into main Aug 2, 2024
5 checks passed
@bparks13 bparks13 deleted the 20240802-docs-update branch August 2, 2024 20:50
@@ -6,19 +6,26 @@ namespace OpenEphys.Onix1
{
/// <summary>
/// Provides an abstract base class for configuration operators responsible for
/// registering all devices in an ONI device aggregate in the context device table.
/// registering logical groups of <see cref="oni.Device"/>s.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This somehow seems now less clear, since it makes it seem the operator is registering the group rather than each individual device. Maybe we shouldn't talk about "device table" since that is a hardware specific concept, but mentioning that there is a context "device manager" might be important so people understand why sometimes the drop down shows devices and sometimes not.

Copy link
Member Author

@jonnew jonnew Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, as far as I see, we are both wrong.

  • Device table is hardware-agnostic, its required by the ONI API regardless of hardware
  • This class does not configure aggregates of oni.Devices (nullifying impact of last bullet), but OpenEphys.Onix1's concept of device, which is much looser (e.g. any dev using passthrough), is really what this class is used to configure. We don't really make it clear that this library kinda cares about devices, but will happily abstract a device if it makes things work better.

I m going to remove references to oni.Devices because its just not correct. I agree with Device manager comment ill try to add some text.

/// These multi-device aggregates are the most common starting point for configuration
/// of an ONI system, and the <see cref="MultiDeviceFactory"/> provides a modular abstraction
/// for flexible assembly and sequencing of multiple such aggregates.
/// This class allows configuration of logical device groups of <see cref="oni.Device"/>s across ONI-defined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This class allows configuration of logical device groups of <see cref="oni.Device"/>s across ONI-defined
/// This class allows configuration of logical groups of <see cref="oni.Device"/>s across ONI-defined

@@ -34,10 +41,10 @@ internal MultiDeviceFactory()
}

/// <summary>
/// Gets or sets a unique hub device name.
/// Gets or sets a unique device group name.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be useful to clarify here that this name is used as a prefix for individual device names in the group.

jonnew added a commit that referenced this pull request Aug 4, 2024
- There was lingering discussion after merge into main occured, this attempts to address that
glopesdev added a commit that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants